-
Notifications
You must be signed in to change notification settings - Fork 17
Improve robustness of t_zero_margin calculation with brentq fallback #4018
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Improve robustness of t_zero_margin calculation with brentq fallback #4018
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4018 +/- ##
==========================================
- Coverage 45.88% 44.06% -1.82%
==========================================
Files 123 123
Lines 29087 30955 +1868
==========================================
+ Hits 13346 13641 +295
- Misses 15741 17314 +1573 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still need to think about this and test it a bit more. This doesn't fix the problem, but I am not so sure there is a solution: we are at a garbage point in the parameter space where a positive solution does not exist.
This PR will stop PROCESS from crashing and instead it will (probably) fail to converge.
The only thing I need to convince myself of is that -1 is an acceptable fallback. Could this -1 end up in the availability model or a constraint, cause that model to be wrong, but we somehow converge?
timothy-nunn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still thinking so no rush with these changes. I think this mixture of local/global is redundant and might even cause the two to be different at points in the execution.
| full_output=True, | ||
| disp=True, | ||
| ) | ||
| temp_tf_superconductor_margin = -1.0e0 # Default value in case of exception |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| temp_tf_superconductor_margin = -1.0e0 # Default value in case of exception | |
Remove this default and use the variable tfcoil_variables.temp_margin, I think having the local variable is redundant
| logger.error( | ||
| """Negative TFC temperature margin | ||
| f"""Negative TFC temperature margin | ||
| temp_tf_superconductor_margin: {temp_tf_superconductor_margin} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| temp_tf_superconductor_margin: {temp_tf_superconductor_margin} | |
| temp_tf_superconductor_margin: {tfcoil_variables.temp_margin} |
| temp_tf_superconductor_margin = t_zero_margin - temp_tf_coolant_peak_field | ||
| tfcoil_variables.temp_margin = temp_tf_superconductor_margin | ||
|
|
||
| if temp_tf_superconductor_margin <= 0.0e0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if temp_tf_superconductor_margin <= 0.0e0: | |
| if tfcoil_variables.temp_margin <= 0.0e0: |
| """ | ||
| ) | ||
|
|
||
| return temp_tf_superconductor_margin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| return tfcoil_variables.temp_margin |
Description
Checklist
I confirm that I have completed the following checks: